Feature/admob 2021 stability in future usage #762

DellaBitta · 2021-11-29T00:05:01Z

Description

Provide details of the change, and generalize the change in the PR title above.

Fixes stability issues regarding race conditions in the AdMob SDK:

Ensure that future objects are fully created before launching async operations which use their handles.
Remove the blocking destructor of BannerView on Android.
- There was a race condition where the the future might be destroyed before the destroy method returns from Android causing an assertion in the Future implementation.
Remove the blocking destructors of Interstitial and Rewarded Ads on iOS.
- These implementations don't invoke AdMob objects and so don't need to be run on the main thread.

Testing

Describe how you've tested these changes. Link any manually triggered Integration tests or CPP binary SDK Packaging Github Action workflows, if applicable.

FTL testing. Local Testing. Integration CI

Type of Change

Place an x the applicable box:

Bug fix. Add the issue # below if applicable.
New feature. A non-breaking change which adds functionality.
Other, such as a build process or documentation change.

DellaBitta · 2021-11-29T16:32:59Z

admob/integration_test/src/integration_test.cc

@@ -795,6 +795,8 @@ TEST_F(FirebaseAdMobTest, TestBannerView) {
  // Set the listener.
  TestBoundingBoxListener bounding_box_listener;
  banner->SetBoundingBoxListener(&bounding_box_listener);
+  PauseForVisualInspectionAndCallbacks();


Wait to ensure that any callback attempts have time to complete before we start testing the listener.

DellaBitta · 2021-11-29T16:38:42Z

admob/src/android/banner_view_internal_android.cc

@@ -390,21 +386,29 @@ Future<void> BannerViewInternalAndroid::Resume() {

 Future<void> BannerViewInternalAndroid::Destroy() {
  firebase::MutexLock lock(mutex_);
-  return InvokeNullary(kBannerViewFnDestroy, banner_view_helper::kDestroy);
+  FutureCallbackData<void>* callback_data =


No longer use futures to track the destruction as the ReferenceCountedFutureImpl may be destroyed before the operation attempts to complete the future. This had caused a crash on some Android runs which was responsible for a lot of flake.

TBH, the Application should have already invoked Destroy() prior to this object's destruction. Invoking it again here isn't required. I guess this code was put here to protect our users from accidentally leaking memory.

In theory this code could be removed, but I'll keep it anyway. But moving forward there doesn't seem to be a good reason to block the destructor and wait, though. Instead I'll start an asynchronous attempt to destroy the Java resources, if they haven't been destroyed already, and then return immediately from here.

Yeah, I think there's no reason to actually wait for the Java destruction to complete.

DellaBitta · 2021-11-29T16:41:26Z

admob/src/common/admob_common.cc

@@ -227,7 +227,9 @@ const char* GetRequestAgentString() {
 // Mark a future as complete.
 void CompleteFuture(int error, const char* error_msg,
                    SafeFutureHandle<void> handle, FutureData* future_data) {
-  future_data->future_impl.Complete(handle, error, error_msg);
+  if (future_data->future_impl.ValidFuture(handle)) {
+    future_data->future_impl.Complete(handle, error, error_msg);


Just an extra safety check.

DellaBitta · 2021-11-29T16:42:52Z

admob/src/ios/interstitial_ad_internal_ios.mm

@@ -32,29 +32,20 @@

 InterstitialAdInternalIOS::~InterstitialAdInternalIOS() {
  firebase::MutexLock lock(mutex_);
-  // Clean up any resources created in InterstitialAdInternalIOS.


This code was waiting to run on the main thread but all it was doing was setting values to null. While all AdMob operations should be conducted on the main thread, we're not actually performing any AdMob operations here, so let's simplify this code and just assign the null values here.

DellaBitta · 2021-11-29T16:43:14Z

admob/src/ios/rewarded_ad_internal_ios.mm

@@ -33,22 +33,13 @@
 RewardedAdInternalIOS::~RewardedAdInternalIOS() {
  firebase::MutexLock lock(mutex_);
  // Clean up any resources created in RewardedAdInternalIOS.
-  Mutex mutex(Mutex::kModeNonRecursive);


Simplify in the same way that we did for Interstitials, above.

DellaBitta · 2021-11-29T16:48:36Z

admob/src_java/com/google/firebase/admob/internal/cpp/BannerViewHelper.java

+              }
+              synchronized (mBannerViewLock) {
+                if (destructor_invocation == false) {
+                  notifyBoundingBoxChanged(mBannerViewInternalPtr);


Don't invoke the bounding box listener if this destroy() invocation originated from the C++ object destructor. The bounding box listener won't be there anymore.

jonsimantov · 2021-11-29T18:18:51Z

admob/src/android/banner_view_internal_android.cc

@@ -390,21 +386,29 @@ Future<void> BannerViewInternalAndroid::Resume() {

 Future<void> BannerViewInternalAndroid::Destroy() {
  firebase::MutexLock lock(mutex_);
-  return InvokeNullary(kBannerViewFnDestroy, banner_view_helper::kDestroy);
+  FutureCallbackData<void>* callback_data =


Yeah, I think there's no reason to actually wait for the Java destruction to complete.

github-actions · 2021-11-29T20:07:20Z

❌ Integration test FAILED

Requested by @DellaBitta on commit b495952
Last updated: Mon Nov 29 14:50 PST 2021
View integration test log & download artifacts

Failures	Configs
firestore	[TEST] [ERROR] [Android] [windows] [emulator_target]
messaging	[TEST] [ERROR] [Android] [ubuntu] [android_target]

Add flaky tests to go/fpl-cpp-flake-tracker

DellaBitta added 7 commits November 27, 2021 08:47

construct future objects before calling async methods

4eabdf0

remove async nature of bannerview destructor

b4251c9

Errant character in BannerViewHelper.java

6673e59

misuse of Android Studio formatter

e2cea9d

support destroy from destructor and diret invocation

c03e414

Java changes

591a658

future callback fix in Java code

40ef248

google-cla bot added the cla: yes label Nov 29, 2021

DellaBitta added 5 commits November 28, 2021 20:17

line lengh in bannerview android header

9c19e08

reduce the complexity of interstitial destructor on iOS

d73c995

Merge branch 'feature/admob_2021' into feature/admob_2021_make_future

37be813

reduce complexity of rewarded ads destructor on iOS

7c1c6cf

Pause to ensure the bounding box listener isn't called

ef47ba6

DellaBitta changed the title ~~Feature/admob 2021 make future~~ Feature/admob 2021 stability in future usage Nov 29, 2021

DellaBitta marked this pull request as ready for review November 29, 2021 14:37

DellaBitta commented Nov 29, 2021

View reviewed changes

DellaBitta added 2 commits November 29, 2021 11:44

BannerViewHelper.java formatting

e79a87f

errant character

5cd408e

DellaBitta commented Nov 29, 2021

View reviewed changes

jonsimantov approved these changes Nov 29, 2021

View reviewed changes

DellaBitta merged commit b495952 into feature/admob_2021 Nov 29, 2021

DellaBitta deleted the feature/admob_2021_make_future branch November 29, 2021 20:07

github-actions bot added the tests: in-progress This PR's integration tests are in progress. label Nov 29, 2021

github-actions bot added the tests: failed This PR's integration tests failed. label Nov 29, 2021

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Nov 29, 2021

firebase locked and limited conversation to collaborators Dec 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feature/admob 2021 stability in future usage #762

Feature/admob 2021 stability in future usage #762

Uh oh!

DellaBitta commented Nov 29, 2021 •

edited

Loading

Uh oh!

DellaBitta Nov 29, 2021

Uh oh!

DellaBitta Nov 29, 2021 •

edited

Loading

Uh oh!

jonsimantov Nov 29, 2021

Uh oh!

DellaBitta Nov 29, 2021

Uh oh!

DellaBitta Nov 29, 2021

Uh oh!

DellaBitta Nov 29, 2021

Uh oh!

DellaBitta Nov 29, 2021

Uh oh!

jonsimantov Nov 29, 2021

Uh oh!

github-actions bot commented Nov 29, 2021 •

edited

Loading

Uh oh!

Uh oh!

Feature/admob 2021 stability in future usage #762

Feature/admob 2021 stability in future usage #762

Uh oh!

Conversation

DellaBitta commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Type of Change

Uh oh!

DellaBitta Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

DellaBitta Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonsimantov Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

DellaBitta Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

DellaBitta Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

DellaBitta Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

DellaBitta Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

jonsimantov Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Integration test FAILED

Uh oh!

Uh oh!

DellaBitta commented Nov 29, 2021 •

edited

Loading

DellaBitta Nov 29, 2021 •

edited

Loading

github-actions bot commented Nov 29, 2021 •

edited

Loading